Skip to content

Fixing issue where total-records was 0 on POST and PATCH calls. #308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 23, 2018

Conversation

nwise
Copy link
Contributor

@nwise nwise commented Jun 15, 2018

Closes #307

BUG FIX

  • reproduce issue in tests
  • fix issue
  • bump package version


return new PageManager
{
DefaultPageSize = Options.DefaultPageSize,
CurrentPage = query.PageOffset,
TotalRecords = (requestMethod == "POST" || requestMethod == "PATCH") ? 1 : 0,
Copy link
Contributor

@jaredcnance jaredcnance Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this changes the semantics of TotalRecords a bit. For GET requests, TotalRecords indicates the number of records in the database and not the number of records in the response, this would change the meaning of the field for POST and PATCH requests.

Do you find this field useful for create/update requests? I wonder if it should simply be excluded.

@nwise
Copy link
Contributor Author

nwise commented Jun 17, 2018

Yeah, that makes sense. I'd prefer that, if the total-records value is included, it should be accurate. What do you think about simply not adding it on PATCH/POST responses? I think that would be simplest and also not require an extra call to the DB.

@jaredcnance
Copy link
Contributor

Yeah I 100% agree. I think we should exclude it in those cases.

@milosloub
Copy link
Contributor

milosloub commented Jun 18, 2018

I think this is good idea. Property TotalRecords corresponds to amount of returned data in case of filtering. So in case of POST and PATCH is good idea to set TotalRecords : 1. It trully returns only one record.

@jaredcnance
Copy link
Contributor

@milosloub the use case TotalRecords is intended to support is pagination.

corresponds to amount of returned data in case of filtering

This is not entirely accurate. It should return the number of records in the database that meet the specified query before pagination, not the number of results in the response payload. (I recognize that our documentation is a bit lacking here)

I am failing to see a use case for including TotalRecords in POST/PATCH requests. Can you elaborate on how this might be used?

@milosloub
Copy link
Contributor

I'm sorry. I didn't notice

Yeah I 100% agree. I think we should exclude it in those cases.

I think this is better solution to exlude in all irrelevant cases. My suggestion was for "total-record" always presented

@jaredcnance
Copy link
Contributor

I think we can simply make TotalRecords int? and change the following line from:

if (_jsonApiContext.Options.IncludeTotalRecordCount)
builder.Add("total-records", _jsonApiContext.PageManager.TotalRecords);

to:

if (_jsonApiContext.Options.IncludeTotalRecordCount && _jsonApiContext.PageManager.TotalRecords != null) 
  builder.Add( /* ... */ )

What this implies is that it will only be included if we have explicitly set it earlier in the pipeline.

@jaredcnance
Copy link
Contributor

jaredcnance commented Jun 21, 2018

Hey guys, take a look at the changes and let me know what you think. I've modified it so even if the payload returns empty, we will still include the total-records meta attribute. This is so that a client application that uses paging doesn't have to first check to make sure total-records is actually set.

Example:

GET /api/articles HTTP/1.1
Accept: application/vnd.api+json
HTTP/1.1 200 OK
Content-Type: application/vnd.api+json

{ "data": [], "meta": { "total-records":0 } }

@nwise
Copy link
Contributor Author

nwise commented Jun 23, 2018

I think this looks good. I like that it's always included on empty results too. It will make coding clients against it simpler. Thanks!

@jaredcnance jaredcnance merged commit 9013643 into json-api-dotnet:master Jun 23, 2018
jaredcnance added a commit that referenced this pull request Aug 12, 2018
Fixing issue where total-records was 0 on POST and PATCH calls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

total-records on POST and PATCH methods
4 participants